ORC-817: Replace aircompressor ZStandard compression with zstd-jni #988

dchristle · 2022-01-03T01:01:11Z

What changes were proposed in this pull request?

This PR proposes to replace the aircompressor library for ORC's ZStandard compression with zstd-jni, which is a set of JNI bindings around the official zstd library. In addition to switching the underlying library, this PR also exposes the compression level and "long mode" settings to ORC users. These settings allow user choice around different speed/compression tradeoffs, rather than the current approach that primarily uses a default setting.

Why are the changes needed?

These change makes sense for a few reasons:

ORC users will gain all the improvements from the main zstd library. It is under active development and receives regular speed and compression improvements. In contrast, aircompressor's zstd implementation is older and stale.
ORC users will be able to use the entire speed/compression tradeoff space. Today, aircompressor's implementation has only one of eight compression strategies (link). This means only a small range of faster but less compressive strategies can be exposed to ORC users. ORC storage with high compression (e.g. for large-but-infrequently-used data) is a clear use case that this PR would unlock.
It will harmonize the Java ORC implementation with other projects in the Hadoop ecosystem. Parquet, Spark, and even the C++ ORC reader/writers all rely on the official zstd implementation either via zstd-jni or directly. In this way, the Java reader/writer code is an outlier.
Detection and fixing any bugs or regressions will generally happen much faster, given the larger number of users and active developer community of zstd and zstd-jni.

The largest tradeoff is that zstd-jni wraps compiled code. That said, many microprocessor architectures are already targeted & bundled into zstd-jni, so this should be a rare hurdle.

Open issues:

What is the best way to expose codec-specific options to users? In this PR, we add the compression level, window log size, and a boolean for enabling long mode, as new conf settings. But the CompressionCodec interface seems limited to exposing an enum with 3 options for speed, e.g. FAST or DEFAULT, and other codec-specific configs don't have a clear way to make it down into the codec implementation itself. I think we want to allow users to set the actual level as an integer, and to specify the window log size & long mode boolean as they wish. It wasn't clear how I could communicate these confs down to the lower level ZstdCodec within the bounds of the existing CompressionCodec interface. Right now, I used a hack to get the codec to read the conf options.
I still need to implement the DirectByteBuffer handling case. Right now, each call is treated the same way and will incur unnecessary copying if the input ByteBuffer is direct.
The existing code has a loop structure to repeatedly decompress. I wasn't sure why this exists, but we should mimic it in this PR, and I haven't done that yet.
Benchmarks should be added to this PR description. Although zstd-jni should have superior performance across the board, it's important to actually measure that with the benchmark suite.

List of changes:

Add zstd-jni dependency, and add a new CompressionCodec ZstdCodec that uses it. Add ORC conf to set compression level.
Add ORC conf to use long mode, and add configuration setters for windowLog and longModeEnable.
Add tests that verify the correctness of writing and reading across compression levels, window sizes, and long mode use.
Add test for compatibility between Zstd aircompressor and zstd-jni implementations.
Fix filterWithSeek test with a smaller percentage.
Minor formatting and spelling fixes.

How was this patch tested?

Unit tests for reading and writing ORC files using a variety of compression levels, window logs, and long mode booleans, all pass.
Unit test to compress and decompress between aircompressor and zstd-jni passes. Note that the current aircompressor implementation uses a small subset of levels, so the test only compares data using the default compression settings.

@dongjoon-hyun @wgtmac

* Add zstd-jni dependency, and add a new CompressionCodec ZstdCodec that uses it. Add ORC conf to set compression level. * Add ORC conf to use long mode, and add configuration setters for windowLog and longModeEnable. * Add tests that verify the correctness of writing and reading across compression levels, window sizes, and long mode use. * Add test for compatibility between Zstd aircompressor and zstd-jni implementations. * Fix filterWithSeek test with a smaller percentage.

dongjoon-hyun

This should be extremely carefully handled, @dchristle .
I'm not considering this at Apache ORC 1.8.0 timeframe.

dongjoon-hyun

Please provide a performance benchmark result at least.

omalley · 2022-01-04T22:15:36Z

We need to leave the pure java library as an option with automatic fallback because not every platform will have the native library. Furthermore, if there is a bug in the aircompresssor codec, we may need to configure it for reading old files.

I also second Dongjoon's concern about benchmarks. JNI can have a positive impact, but it can also have negative impacts. That is especially true if non-native ByteBuffers (or byte arrays) are used.

github-actions · 2023-01-05T00:06:34Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

tomscut · 2023-07-25T06:02:04Z

Hi @dchristle , do you have a performance benchmark result? Thanks.

cxzl25 · 2024-01-10T09:04:07Z

I did some tests based on this PR. Under the same Spark resources, zstd-jni compression is faster than aircompressor.

Here is also a performance comparison. opensearch-project/OpenSearch#9422 (comment)

spark

spark.executor.instances=600
spark.executor.cores=3

use aircompressor

use zstd-jni

wgtmac · 2024-01-11T07:22:27Z

Thanks for providing the benchmark result. @cxzl25

As @omalley has commented, it would be good to use zstd-jni by default but we have to keep the pure java impl as a fallback.

cxzl25 · 2024-01-11T08:55:15Z

it would be good to use zstd-jni by default but we have to keep the pure java impl as a fallback.

I think we should be able to achieve this by automatically fallback, such as the following code,
or providing configuration items, so that the user can specify the implementation of a certain zstd.

org.apache.orc.impl.WriterImpl

  static {
    try {
      Native.load();
    } catch (UnsatisfiedLinkError | ExceptionInInitializerError e) {
    }
  }

  public static CompressionCodec createCodec(CompressionKind kind) {
    ...
      case ZSTD:
        if (Native.isLoaded()) {
          return new ZstdCodec();
        } else {
          return new AircompressorCodec(kind, new ZstdCompressor(),
                  new ZstdDecompressor());
        }

dongjoon-hyun · 2024-01-11T10:17:20Z

+1 for that fallback.

Could you take over this PR, @cxzl25 ?
I can wait for your PR to include it to Apache ORC 2.0.0.

dongjoon-hyun · 2024-01-11T10:20:26Z

BTW, we need more time to test because zstd-jni has memory issues depending on its usage.
IIRC, it was reported in Parquet and Spark community before. It's hard to detect memory-leak.

### What changes were proposed in this pull request? Original PR: #988 Original author: dchristle This PR will support the use of [zstd-jni](https://github.com/luben/zstd-jni) library as the implementation of ORC zstd, with better performance than [aircompressor](https://github.com/airlift/aircompressor). (#988 (comment)) This PR also exposes the compression level and "long mode" settings to ORC users. These settings allow the user to select different speed/compression trade-offs that were not supported by the original aircompressor. - Add zstd-jni dependency, and add a new CompressionCodec ZstdCodec that uses it. Add ORC conf to set compression level. - Add ORC conf to use long mode, and add configuration setters for windowLog. - Add tests that verify the correctness of writing and reading across compression levels, window sizes, and long mode use. - Add test for compatibility between Zstd aircompressor and zstd-jni implementations. ### Why are the changes needed? These change makes sense for a few reasons: ORC users will gain all the improvements from the main zstd library. It is under active development and receives regular speed and compression improvements. In contrast, aircompressor's zstd implementation is older and stale. ORC users will be able to use the entire speed/compression tradeoff space. Today, aircompressor's implementation has only one of eight compression strategies ([link](https://github.com/airlift/aircompressor/blob/c5e6972bd37e1d3834514957447028060a268eea/src/main/java/io/airlift/compress/zstd/CompressionParameters.java#L143)). This means only a small range of faster but less compressive strategies can be exposed to ORC users. ORC storage with high compression (e.g. for large-but-infrequently-used data) is a clear use case that this PR would unlock. It will harmonize the Java ORC implementation with other projects in the Hadoop ecosystem. Parquet, Spark, and even the C++ ORC reader/writers all rely on the official zstd implementation either via zstd-jni or directly. In this way, the Java reader/writer code is an outlier. Detection and fixing any bugs or regressions will generally happen much faster, given the larger number of users and active developer community of zstd and zstd-jni. The largest tradeoff is that zstd-jni wraps compiled code. That said, many microprocessor architectures are already targeted & bundled into zstd-jni, so this should be a rare hurdle. ### How was this patch tested? - Unit tests for reading and writing ORC files using a variety of compression levels, window logs, all pass. - Unit test to compress and decompress between aircompressor and zstd-jni passes. Note that the current aircompressor implementation uses a small subset of levels, so the test only compares data using the default compression settings. ### Was this patch authored or co-authored using generative AI tooling? No Closes #1743 from cxzl25/ORC-817. Lead-authored-by: sychen <[email protected]> Co-authored-by: Dongjoon Hyun <[email protected]> Co-authored-by: David Christle <[email protected]> Co-authored-by: Yiqun Zhang <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

### What changes were proposed in this pull request? Original PR: #988 Original author: dchristle This PR will support the use of [zstd-jni](https://github.com/luben/zstd-jni) library as the implementation of ORC zstd, with better performance than [aircompressor](https://github.com/airlift/aircompressor). (#988 (comment)) This PR also exposes the compression level and "long mode" settings to ORC users. These settings allow the user to select different speed/compression trade-offs that were not supported by the original aircompressor. - Add zstd-jni dependency, and add a new CompressionCodec ZstdCodec that uses it. Add ORC conf to set compression level. - Add ORC conf to use long mode, and add configuration setters for windowLog. - Add tests that verify the correctness of writing and reading across compression levels, window sizes, and long mode use. - Add test for compatibility between Zstd aircompressor and zstd-jni implementations. ### Why are the changes needed? These change makes sense for a few reasons: ORC users will gain all the improvements from the main zstd library. It is under active development and receives regular speed and compression improvements. In contrast, aircompressor's zstd implementation is older and stale. ORC users will be able to use the entire speed/compression tradeoff space. Today, aircompressor's implementation has only one of eight compression strategies ([link](https://github.com/airlift/aircompressor/blob/c5e6972bd37e1d3834514957447028060a268eea/src/main/java/io/airlift/compress/zstd/CompressionParameters.java#L143)). This means only a small range of faster but less compressive strategies can be exposed to ORC users. ORC storage with high compression (e.g. for large-but-infrequently-used data) is a clear use case that this PR would unlock. It will harmonize the Java ORC implementation with other projects in the Hadoop ecosystem. Parquet, Spark, and even the C++ ORC reader/writers all rely on the official zstd implementation either via zstd-jni or directly. In this way, the Java reader/writer code is an outlier. Detection and fixing any bugs or regressions will generally happen much faster, given the larger number of users and active developer community of zstd and zstd-jni. The largest tradeoff is that zstd-jni wraps compiled code. That said, many microprocessor architectures are already targeted & bundled into zstd-jni, so this should be a rare hurdle. ### How was this patch tested? - Unit tests for reading and writing ORC files using a variety of compression levels, window logs, all pass. - Unit test to compress and decompress between aircompressor and zstd-jni passes. Note that the current aircompressor implementation uses a small subset of levels, so the test only compares data using the default compression settings. ### Was this patch authored or co-authored using generative AI tooling? No Closes #1743 from cxzl25/ORC-817. Lead-authored-by: sychen <[email protected]> Co-authored-by: Dongjoon Hyun <[email protected]> Co-authored-by: David Christle <[email protected]> Co-authored-by: Yiqun Zhang <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit 33be571) Signed-off-by: Dongjoon Hyun <[email protected]>

github-actions bot added BUILD JAVA labels Jan 3, 2022

dongjoon-hyun reviewed Jan 3, 2022

View reviewed changes

dongjoon-hyun requested changes Jan 3, 2022

View reviewed changes

github-actions bot added the Stale label Jan 5, 2023

github-actions bot closed this Jan 6, 2023

cxzl25 mentioned this pull request Jan 11, 2024

ORC-817, ORC-1088: Support ZStandard compression using zstd-jni #1743

Closed

zyl891229 mentioned this pull request Mar 20, 2024

[Format]ORC Support ZStandard compression using zstd-jni apache/paimon#3056

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ORC-817: Replace aircompressor ZStandard compression with zstd-jni #988

ORC-817: Replace aircompressor ZStandard compression with zstd-jni #988

dchristle commented Jan 3, 2022 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading

dongjoon-hyun left a comment

omalley commented Jan 4, 2022

github-actions bot commented Jan 5, 2023

tomscut commented Jul 25, 2023

cxzl25 commented Jan 10, 2024

wgtmac commented Jan 11, 2024

cxzl25 commented Jan 11, 2024

dongjoon-hyun commented Jan 11, 2024

dongjoon-hyun commented Jan 11, 2024

ORC-817: Replace aircompressor ZStandard compression with zstd-jni #988

ORC-817: Replace aircompressor ZStandard compression with zstd-jni #988

Conversation

dchristle commented Jan 3, 2022 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Open issues:

How was this patch tested?

dongjoon-hyun left a comment • edited Loading

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

omalley commented Jan 4, 2022

github-actions bot commented Jan 5, 2023

tomscut commented Jul 25, 2023

cxzl25 commented Jan 10, 2024

spark

use aircompressor

use zstd-jni

wgtmac commented Jan 11, 2024

cxzl25 commented Jan 11, 2024

dongjoon-hyun commented Jan 11, 2024

dongjoon-hyun commented Jan 11, 2024

dchristle commented Jan 3, 2022 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading